Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependencies and fix ESLint warnings accordingly #2433

Merged
merged 8 commits into from Feb 1, 2018
Merged

Update dependencies and fix ESLint warnings accordingly #2433

merged 8 commits into from Feb 1, 2018

Conversation

igor-savin-ht
Copy link
Contributor

No description provided.

@igor-savin-ht
Copy link
Contributor Author

@elhigu Would be great if you could take a look at aaa4d71 -> I've removed it as ESLint demanded me to do so, but I wonder if there was actually a bug there and that param was supposed to be used.

@igor-savin-ht
Copy link
Contributor Author

@elhigu I can restore removed code with eslint ignore and add the ToDo to review it later, if it's not an option to check it now :)

@elhigu
Copy link
Member

elhigu commented Jan 29, 2018

@igor-savin-ht honestly I just haven't got time to read this through with thought. Smaller changes are easier to accept with confidence that they are not breaking anything :) I would prefer eslint ignore for now.

@igor-savin-ht
Copy link
Contributor Author

@elhigu Done!

@elhigu
Copy link
Member

elhigu commented Jan 30, 2018

There were few regex where some escape backslashes were dropped. Are you sure they work exactly the same after the changes?

@igor-savin-ht
Copy link
Contributor Author

@elhigu ESLint was complaining about those escape backslashes being redundant, I tend to trust its judgement on that. Here is an in-depth discussion on this subject: eslint/eslint#7656
As is with unused variable, I'm open to replacing this change with either line-specific ignore or global rule to ignore useless escapes.

@elhigu
Copy link
Member

elhigu commented Jan 31, 2018

I'd rather have that global rule set so I need to verify manually e.g. with runkit that each of those regexps are working as expected after change. In this case I don't trust knex testsuite enough and I believe that eslint has them correctly fixed, but I need to be sure.

@@ -153,7 +153,7 @@ function convertTimezone(tz) {
if (tz === 'Z') {
return 0;
}
const m = tz.match(/([\+\-\s])(\d\d):?(\d\d)?/);
const m = tz.match(/([+\-\s])(\d\d):?(\d\d)?/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I wonder why eslint left \-escaping there.

@elhigu
Copy link
Member

elhigu commented Feb 1, 2018

All good https://runkit.com/embed/ygveesp9rad2

@elhigu elhigu merged commit 09eb126 into knex:master Feb 1, 2018
joelmukuthu pushed a commit to joelmukuthu/knex that referenced this pull request Feb 16, 2018
* Update dependencies. Tweak ESLint rules to work more like they used to before

* Fix indentation

* Remove unnecessary escapes.

* Remove unused 'usingClause' parameter.

* Address CI failures.

* Revert "Remove unused 'usingClause' parameter."

This reverts commit aaa4d71.

* eslint: add ignore with todo

* dependencies: update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants